I believe I've verified that my fix works (local server can be used to pass the Alamofire test that previously hung), but I still need guidance around the proper conditions and unit test.

Proper way to generalize the write(data:) implementation. Right now I just check the decoder's full metatype. What's the preferred pattern here?

You can use HTTPDecoder.kind, which tells you which kind of decoder you are.

Proper condition for reenabling parsing. Right now I ensure that the connection will be kept alive (if it's being killed who cares), that the response is NOT an upgrade, and that it has stopped parsing.

I think for now this is appropriate.

Proper test. I see the HTTPDecoderTest but it's not clear what kind of test would be best here, or how to simulate this scenario exactly.

I think you want a few scenarios:

1. Send an upgrade request, do not accept the upgrade, and then send another request. Confirm both get through.
2. Pipeline a pair of requests. This is generally unsound and we don't really support it, but if the upgrade fails then we should resume parsing the second one. (I have an inkling this test will fail, it's possible that we need to trigger ourselves to start parsing again in this case which will be a fun wrinkle).

To achieve "do not accept the upgrade" you don't actually need an encoder, you can just write the response head into the pipeline. EmbeddedChannel doesn't care what data you pass through it.

@swift-nio-bot test performance please

@swift-nio-bot performance test please

@swift-server-bot test performance please

@swift-server-bot add to allowlist

@swift-server-bot test perf please

performance report

build id: 151

timestamp: Thu Oct 26 07:07:05 UTC 2023

results

comparison

significant differences found

@Lukasa / @jshier ugh, this is rough

no-net_http1_1k_reqs_1_conn | 0.013450951 | 0.011678896 | previous | 15%

15% slower with this change :|. And that includes client (unchanged) & servers (changed) meaning that servers will likely be affected even more gravely.

The change as written does runtime type checks. My review feedback above would replace it with a jump based on an enum value, which should perform a lot better.

The change as written does runtime type checks. My review feedback above would replace it with a jump based on an enum value, which should perform a lot better.

Missed that, sorry. That's great!

I've pushed the kind check up, feel free to test that, but I think there will still be an impact due to the data cast. I'll try to finish up the local tests soon.

@swift-server-bot test perf please

!!! Couldn't read commit file !!!

@swift-server-bot test perf please

!!! Couldn't read commit file !!!

I've added two tests, one for successful upgrade and one for failed upgrade.

@Lukasa What does a pipelined request pair look like? Do I just shove both through in one buffer?

What does a pipelined request pair look like? Do I just shove both through in one buffer?

Yeah, that's the easiest way to do it.

@Lukasa I can write a pipelined test for the current behavior just fine, as parsing is stopped as soon as the first request is parsed, but I'm not clear what behavior I should be seeing in the unsuccessful upgrade case. As far as I can tell, the data just disappears, and the channel is empty. I'm guessing that with parsing stopped, the additional data is just lost, and the unsuccessful upgrade turns parsing back on for a decoder that doesn't receive any additional data. To support the pipeline case I think I'd need to buffer any additional requests until we can tell whether the upgrade was successful, or some other change in behavior.

I don't think the data is lost, I think you need a way to re-spin the decoder. To test, can you try amending your current test case for the pipeline to send an extra empty ByteBuffer after you send the response that should re-enable parsing? My guess is that this will appear to "work", in which case my analysis is right.

@Lukasa You're correct, sending an empty buffer lets the test pass. Instead of just setting stopParsing = false, I'll also need to restart the parsing process.

@Lukasa I've investigated how to restart parsing in HTTPDecoder and there doesn't seem to be any way to do so simply. channel.writeInbound triggers a codepath the decoder can't reach, and since the decoder nils out the context when reading finishes, there's no way to start reading again internally. We could make the niling behavior dependent on the outgoing response, but I don't know if that's appropriate here. Any ideas what a proper fix might be here?